adding feature flag on dependency level #1801
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds feature flag support at the dependency level, enabling tools to conditionally alter their behavior based on runtime feature flags. A new IsFeatureEnabled method is added to ToolDependencies, along with a HelloWorld demonstration tool that changes its greeting based on the remote_mcp_experimental feature flag.
Changes:
- Added
IsFeatureEnabledmethod toToolDependenciesinterface andBaseDepsimplementation - Created
HelloWorldtool demonstrating feature flag conditional behavior - Updated
NewBaseDepsconstructor to accept afeatureCheckerparameter - Added
RemoteMCPExperimentalfeature flag constant
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/dependencies.go | Adds IsFeatureEnabled method to interface and implementation, updates NewBaseDeps signature |
| pkg/github/hello.go | New demonstration tool showing feature flag conditional behavior |
| pkg/github/hello_test.go | Comprehensive tests for HelloWorld tool with feature flag scenarios |
| pkg/github/dependencies_test.go | Tests for IsFeatureEnabled covering various edge cases |
| pkg/github/feature_flags.go | Adds RemoteMCPExperimental constant for feature flag name |
| internal/ghmcp/server.go | Wires feature checker into BaseDeps creation |
| pkg/github/server_test.go | Updates stub to implement new interface method |
| pkg/github/dynamic_tools_test.go | Updates NewBaseDeps call with nil checker |
| enabled, err := d.featureChecker(ctx, flagName) | ||
| if err != nil { | ||
| // Log error but don't fail the tool - treat as disabled | ||
| fmt.Fprintf(os.Stderr, "Feature flag check error for %q: %v\n", flagName, err) |
There was a problem hiding this comment.
The error logging uses fmt.Fprintf to os.Stderr directly instead of using the project's logging infrastructure (pkg/log). This is inconsistent with how errors are logged elsewhere in the codebase. Consider using the established logging package for consistent error handling and formatting.
Summary
Currently our feature flag allow toggling tools to enabled/disabled. Adding it to deps allows us to change code inside the tools. Adding hello world example to demonstrate a tool behaviour change.
Why
Fixes #
What changed
MCP impact
Prompts tested (tool changes only)
Security / limits
Tool renaming
deprecated_tool_aliases.goNote: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.
Lint & tests
./script/lint./script/testDocs